fix(*) use dedicated shm for rate-limiting plugins #3311

thibaultcha · 2018-03-17T08:57:51Z

This is part of a series of fixes:

Context

In the local mode of the rate-limiting plugins, storing the
rate-limiting counters in the same shm used by Kong's database cache is
too invasive for the underlying shm, especially when the rate-limiting
plugins are used with a seconds precision.

On top of exhausting the database cache slots, this approach also
generates some form of fragmentation in the shm. This is due to the
side-by-side storage of values with sizes of different orders of
magnitude (JSON strings vs. an incremented double) and the LRU eviction
mechanism. When the shm is full and LRU kicks-in, it is highly probable
that several rate-limiting counters will be evicted (due to their
proliferation), thus not freeing enough space to store the retrieved
data, causing a no memory error to be reported by the shm.

Solution

Declaring shms that are only used by some plugins is not very elegant.
Now, all users (even those not using rate-limiting plugins) have to pay
a memory cost (although small).
Unfortunately, and in the absence of a more dynamic solution to shm
configuration such as a more dynamic templating engine, or a
configure_by_lua phase, this is the safest solution.

Size rationale

Running a script generating similar keys and storing similar values
(double) indicates that an shm with 12Mb should be able to store about
~48,000 of those values at once. It is important to remind ourselves
that one Consumer/IP address might use more than one key (in fact, one
per period configured on the plugin), and both the rate-limiting and
response-ratelimiting plugins at once, and they use the same shms.

Even considering the above statements, ~48,000 keys per node seems
somewhat reasonable, considering keys of second precision will most
likely fill up the shm and be candidates for LRU eviction. Our concern
lies instead around long-lived limits (and thus, keys) set by the user.

Additionally, a future improvement upon this will be the setting of the
init_ttl argument for the rate-limiting keys, which will help quite
considerably in reducing the footprint of the plugins on the shm. As
of this day, this feature has been contributed to ngx_lua but not
released yet:

openresty/lua-nginx-module#1226

I am sure you thought about it a lot, but this feature seems to make rate-limiting plugin a core feature, while it is still a plugin. It feels a bit bad that plugins inject stuff in core.

Yep. This is also why the fix took so long to appear... We have little choice for now though.

I'd like kong_rate_limiting as dict name better as it pollutes core a bit less.

We can do that. Probably kong_rate_limiting_counter for completesness's sake then.

bungle · 2018-03-19T21:05:45Z

kong/plugins/rate-limiting/policies/init.lua

@@ -4,7 +4,7 @@ local redis = require "resty.redis"
 local policy_cluster = require "kong.plugins.rate-limiting.policies.cluster"
 local reports = require "kong.core.reports"
 local ngx_log = ngx.log
-local shm = ngx.shared.kong_cache
+local shm = ngx.shared.kong_rl_counters


Other option could be adding a new config parameter to rate-limiting plugin where you can configure the shm per plugin configuration. Not sure though, should we still supply a default.

With configure_by_lua, yep! Otherwise, it becomes harder to maintain a custom nginx configuration in sync with the plugins configured in the database...

(That or a more powerful templating system that we do not have and tbh, aren't sure of ever wanting at all, as it would make custom nginx templates harder and harder to maintain...)

This is part of a series of fixes: - thibaultcha/lua-resty-mlcache#41 - thibaultcha/lua-resty-mlcache#42 - #3311 - #3341 Context ------- In the `local` mode of the rate-limiting plugins, storing the rate-limiting counters in the same shm used by Kong's database cache is too invasive for the underlying shm, especially when the rate-limiting plugins are used with a `seconds` precision. On top of exhausting the database cache slots, this approach also generates some form of fragmentation in the shm. This is due to the side-by-side storage of values with sizes of different orders of magnitude (JSON strings vs. an incremented double) and the LRU eviction mechanism. When the shm is full and LRU kicks-in, it is highly probable that several rate-limiting counters will be evicted (due to their proliferation), thus not freeing enough space to store the retrieved data, causing a `no memory` error to be reported by the shm. Solution -------- Declaring shms that are only used by some plugins is not very elegant. Now, all users (even those not using rate-limiting plugins) have to pay a memory cost (although small). Unfortunately, and in the absence of a more dynamic solution to shm configuration such as a more dynamic templating engine, or a `configure_by_lua` phase, this is the safest solution. Size rationale -------------- Running a script generating similar keys and storing similar values (double) indicates that an shm with 12Mb should be able to store about ~48,000 of those values at once. It is important to remind ourselves that one Consumer/IP address might use more than one key (in fact, one per period configured on the plugin), and both the rate-limiting and response-ratelimiting plugins at once, and they use the same shms. Even considering the above statements, ~48,000 keys per node seems somewhat reasonable, considering keys of `second` precision will most likely fill up the shm and be candidates for LRU eviction. Our concern lies instead around long-lived limits (and thus, keys) set by the user. Additionally, a future improvement upon this will be the setting of the `init_ttl` argument for the rate-limiting keys, which will help **quite considerably** in reducing the footprint of the plugins on the shm. As of this day, this feature has been contributed to ngx_lua but not released yet: openresty/lua-nginx-module#1226 Again, this limit only applies when using the **local** strategy, which also likely means that a load-balancer is distributing traffic to a pool of Kong nodes with some sort of consistent load-balancing technique. Thus considerably reducing the number of concurrent Consumers a given node needs to handle at once. See also -------- Another piece of the fixes for the `no memory` errors resides in the behavior of the database caching module upon a full shm. See: thibaultcha/lua-resty-mlcache#41 This patch reduces the likeliness of a full shm (by a lot!), but does not remove it. The above patch ensures a somewhat still sane behavior would the shm happen to be full again. Fix #3124 Fix #3241

This is part of a series of fixes: - thibaultcha/lua-resty-mlcache#41 - thibaultcha/lua-resty-mlcache#42 - #3311 - #3341 Changes: * remove vendor mlcache.lua file which had received patches for Kong support * add lua-resty-mlcache 2.0.0 as a dependency * implement custom IPC options for the DB mlcache instance Changelog of mlcache 2.0.0: https://github.com/thibaultcha/lua-resty-mlcache/blob/master/CHANGELOG.md#200

Following this mlcache patch: thibaultcha/lua-resty-mlcache#42 We can now specify a different shm for mlcache to cache L3 misses. This is especially helpful in the context of Kong since client-triggered DB lookups can have a very high cardinality of keys to fetch (e.g. credentials such as API keys) and can make the cache turnover so high that it can be rendered almost useless (filled with misses, thus evicting actual hits from the cache shm). This is considered as a potential attack vector. The size of this shm (12MB) allows for roughly ~45,000 nil sentinel values to be stored in the shm (depending on the size of the keys). This value is aligned with that chosen for the rate-limiting shared dict in PR #3311 (12MB and about ~48,000 simultaneous counters).

This is part of a series of fixes: - thibaultcha/lua-resty-mlcache#41 - thibaultcha/lua-resty-mlcache#42 - #3311 - #3341 Changes: * remove vendor mlcache.lua file which had received patches for Kong support * add lua-resty-mlcache 2.0.1 as a dependency * implement custom IPC options for the DB mlcache instance Changelog of mlcache 2.0.0: https://github.com/thibaultcha/lua-resty-mlcache/blob/master/CHANGELOG.md#200 Changelog of mlcache 2.0.1: https://github.com/thibaultcha/lua-resty-mlcache/blob/master/CHANGELOG.md#201

Following this mlcache patch: thibaultcha/lua-resty-mlcache#42 We can now specify a different shm for mlcache to cache L3 misses. This is especially helpful in the context of Kong since client-triggered DB lookups can have a very high cardinality of keys to fetch (e.g. credentials such as API keys) and can make the cache turnover so high that it can be rendered almost useless (filled with misses, thus evicting actual hits from the cache shm). This is considered as a potential attack vector. The size of this shm (12MB) allows for roughly ~45,000 nil sentinel values to be stored in the shm (depending on the size of the keys). This value is aligned with that chosen for the rate-limiting shared dict in PR #3311 (12MB and about ~48,000 simultaneous counters).

This is part of a series of fixes: - thibaultcha/lua-resty-mlcache#41 - thibaultcha/lua-resty-mlcache#42 - #3311 - #3341 Changes: * remove vendor mlcache.lua file which had received patches for Kong support * add lua-resty-mlcache 2.0.1 as a dependency * implement custom IPC options for the DB mlcache instance Changelog of mlcache 2.0.0: https://github.com/thibaultcha/lua-resty-mlcache/blob/master/CHANGELOG.md#200 Changelog of mlcache 2.0.1: https://github.com/thibaultcha/lua-resty-mlcache/blob/master/CHANGELOG.md#201

Following this mlcache patch: thibaultcha/lua-resty-mlcache#42 We can now specify a different shm for mlcache to cache L3 misses. This is especially helpful in the context of Kong since client-triggered DB lookups can have a very high cardinality of keys to fetch (e.g. credentials such as API keys) and can make the cache turnover so high that it can be rendered almost useless (filled with misses, thus evicting actual hits from the cache shm). This is considered as a potential attack vector. The size of this shm (12MB) allows for roughly ~45,000 nil sentinel values to be stored in the shm (depending on the size of the keys). This value is aligned with that chosen for the rate-limiting shared dict in PR #3311 (12MB and about ~48,000 simultaneous counters).

This is part of a series of fixes: - thibaultcha/lua-resty-mlcache#41 - thibaultcha/lua-resty-mlcache#42 - #3311 - #3341 Context ------- In the `local` mode of the rate-limiting plugins, storing the rate-limiting counters in the same shm used by Kong's database cache is too invasive for the underlying shm, especially when the rate-limiting plugins are used with a `seconds` precision. On top of exhausting the database cache slots, this approach also generates some form of fragmentation in the shm. This is due to the side-by-side storage of values with sizes of different orders of magnitude (JSON strings vs. an incremented double) and the LRU eviction mechanism. When the shm is full and LRU kicks-in, it is highly probable that several rate-limiting counters will be evicted (due to their proliferation), thus not freeing enough space to store the retrieved data, causing a `no memory` error to be reported by the shm. Solution -------- Declaring shms that are only used by some plugins is not very elegant. Now, all users (even those not using rate-limiting plugins) have to pay a memory cost (although small). Unfortunately, and in the absence of a more dynamic solution to shm configuration such as a more dynamic templating engine, or a `configure_by_lua` phase, this is the safest solution. Size rationale -------------- Running a script generating similar keys and storing similar values (double) indicates that an shm with 12Mb should be able to store about ~48,000 of those values at once. It is important to remind ourselves that one Consumer/IP address might use more than one key (in fact, one per period configured on the plugin), and both the rate-limiting and response-ratelimiting plugins at once, and they use the same shms. Even considering the above statements, ~48,000 keys per node seems somewhat reasonable, considering keys of `second` precision will most likely fill up the shm and be candidates for LRU eviction. Our concern lies instead around long-lived limits (and thus, keys) set by the user. Additionally, a future improvement upon this will be the setting of the `init_ttl` argument for the rate-limiting keys, which will help **quite considerably** in reducing the footprint of the plugins on the shm. As of this day, this feature has been contributed to ngx_lua but not released yet: openresty/lua-nginx-module#1226 Again, this limit only applies when using the **local** strategy, which also likely means that a load-balancer is distributing traffic to a pool of Kong nodes with some sort of consistent load-balancing technique. Thus considerably reducing the number of concurrent Consumers a given node needs to handle at once. See also -------- Another piece of the fixes for the `no memory` errors resides in the behavior of the database caching module upon a full shm. See: thibaultcha/lua-resty-mlcache#41 This patch reduces the likeliness of a full shm (by a lot!), but does not remove it. The above patch ensures a somewhat still sane behavior would the shm happen to be full again. Fix #3124 Fix #3241 From #3311

This is part of a series of fixes: - thibaultcha/lua-resty-mlcache#41 - thibaultcha/lua-resty-mlcache#42 - #3311 - #3341 Changes: * remove vendor mlcache.lua file which had received patches for Kong support * add lua-resty-mlcache 2.0.1 as a dependency * implement custom IPC options for the DB mlcache instance Changelog of mlcache 2.0.0: https://github.com/thibaultcha/lua-resty-mlcache/blob/master/CHANGELOG.md#200 Changelog of mlcache 2.0.1: https://github.com/thibaultcha/lua-resty-mlcache/blob/master/CHANGELOG.md#201

Following this mlcache patch: thibaultcha/lua-resty-mlcache#42 We can now specify a different shm for mlcache to cache L3 misses. This is especially helpful in the context of Kong since client-triggered DB lookups can have a very high cardinality of keys to fetch (e.g. credentials such as API keys) and can make the cache turnover so high that it can be rendered almost useless (filled with misses, thus evicting actual hits from the cache shm). This is considered as a potential attack vector. The size of this shm (12MB) allows for roughly ~45,000 nil sentinel values to be stored in the shm (depending on the size of the keys). This value is aligned with that chosen for the rate-limiting shared dict in PR #3311 (12MB and about ~48,000 simultaneous counters).

This is part of a series of fixes: - thibaultcha/lua-resty-mlcache#41 - thibaultcha/lua-resty-mlcache#42 - #3311 - #3341 Context ------- In the `local` mode of the rate-limiting plugins, storing the rate-limiting counters in the same shm used by Kong's database cache is too invasive for the underlying shm, especially when the rate-limiting plugins are used with a `seconds` precision. On top of exhausting the database cache slots, this approach also generates some form of fragmentation in the shm. This is due to the side-by-side storage of values with sizes of different orders of magnitude (JSON strings vs. an incremented double) and the LRU eviction mechanism. When the shm is full and LRU kicks-in, it is highly probable that several rate-limiting counters will be evicted (due to their proliferation), thus not freeing enough space to store the retrieved data, causing a `no memory` error to be reported by the shm. Solution -------- Declaring shms that are only used by some plugins is not very elegant. Now, all users (even those not using rate-limiting plugins) have to pay a memory cost (although small). Unfortunately, and in the absence of a more dynamic solution to shm configuration such as a more dynamic templating engine, or a `configure_by_lua` phase, this is the safest solution. Size rationale -------------- Running a script generating similar keys and storing similar values (double) indicates that an shm with 12Mb should be able to store about ~48,000 of those values at once. It is important to remind ourselves that one Consumer/IP address might use more than one key (in fact, one per period configured on the plugin), and both the rate-limiting and response-ratelimiting plugins at once, and they use the same shms. Even considering the above statements, ~48,000 keys per node seems somewhat reasonable, considering keys of `second` precision will most likely fill up the shm and be candidates for LRU eviction. Our concern lies instead around long-lived limits (and thus, keys) set by the user. Additionally, a future improvement upon this will be the setting of the `init_ttl` argument for the rate-limiting keys, which will help **quite considerably** in reducing the footprint of the plugins on the shm. As of this day, this feature has been contributed to ngx_lua but not released yet: openresty/lua-nginx-module#1226 Again, this limit only applies when using the **local** strategy, which also likely means that a load-balancer is distributing traffic to a pool of Kong nodes with some sort of consistent load-balancing technique. Thus considerably reducing the number of concurrent Consumers a given node needs to handle at once. See also -------- Another piece of the fixes for the `no memory` errors resides in the behavior of the database caching module upon a full shm. See: thibaultcha/lua-resty-mlcache#41 This patch reduces the likeliness of a full shm (by a lot!), but does not remove it. The above patch ensures a somewhat still sane behavior would the shm happen to be full again. Fix #3124 Fix #3241 From #3311

This is part of a series of fixes: - thibaultcha/lua-resty-mlcache#41 - thibaultcha/lua-resty-mlcache#42 - #3311 - #3341 Changes: * remove vendor mlcache.lua file which had received patches for Kong support * add lua-resty-mlcache 2.0.1 as a dependency * implement custom IPC options for the DB mlcache instance Changelog of mlcache 2.0.0: https://github.com/thibaultcha/lua-resty-mlcache/blob/master/CHANGELOG.md#200 Changelog of mlcache 2.0.1: https://github.com/thibaultcha/lua-resty-mlcache/blob/master/CHANGELOG.md#201

Following this mlcache patch: thibaultcha/lua-resty-mlcache#42 We can now specify a different shm for mlcache to cache L3 misses. This is especially helpful in the context of Kong since client-triggered DB lookups can have a very high cardinality of keys to fetch (e.g. credentials such as API keys) and can make the cache turnover so high that it can be rendered almost useless (filled with misses, thus evicting actual hits from the cache shm). This is considered as a potential attack vector. The size of this shm (12MB) allows for roughly ~45,000 nil sentinel values to be stored in the shm (depending on the size of the keys). This value is aligned with that chosen for the rate-limiting shared dict in PR #3311 (12MB and about ~48,000 simultaneous counters).

Render newly added shared dicts required. As 0.14 ships with breaking changes and other nginx configuration changes, now is a good time to render recent shared dicts mandatory. See #3550 See #3311

Render newly added shared dicts required. As 0.14 ships with breaking changes and other nginx configuration changes, now is a good time to render recent shared dicts mandatory. See #3550 See #3311 From #3557

thibaultcha force-pushed the fix/rl-reserved-shm branch from 7f6d0d5 to a3ff4b2 Compare March 17, 2018 19:38

thibaultcha added the pr/please review label Mar 17, 2018

bungle reviewed Mar 19, 2018

View reviewed changes

thibaultcha force-pushed the fix/rl-reserved-shm branch from a3ff4b2 to e462456 Compare March 20, 2018 01:12

thibaultcha force-pushed the fix/rl-reserved-shm branch from e462456 to d8f3671 Compare March 26, 2018 22:29

thibaultcha changed the base branch from master to next March 26, 2018 22:29

thibaultcha mentioned this pull request Mar 26, 2018

chore(deps) use upstream mlcache 2.0.0 #3341

Merged

thibaultcha force-pushed the fix/rl-reserved-shm branch from d8f3671 to ce51957 Compare March 26, 2018 22:40

bungle approved these changes Mar 27, 2018

View reviewed changes

bungle added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/please review labels Mar 27, 2018

thibaultcha merged commit b0a5e9c into next Mar 28, 2018

thibaultcha deleted the fix/rl-reserved-shm branch March 28, 2018 17:21

@@ @@ -70,6 +70,7 @@ return { @@
                 DICTS = {
                   "kong",
                   "kong_cache",
+                  "kong_rl_counters",

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(*) use dedicated shm for rate-limiting plugins #3311

fix(*) use dedicated shm for rate-limiting plugins #3311

thibaultcha commented Mar 17, 2018 •

edited

Loading

bungle Mar 19, 2018

thibaultcha Mar 19, 2018

bungle Mar 19, 2018

thibaultcha Mar 19, 2018

thibaultcha Mar 19, 2018

fix(*) use dedicated shm for rate-limiting plugins #3311

fix(*) use dedicated shm for rate-limiting plugins #3311

Conversation

thibaultcha commented Mar 17, 2018 • edited Loading

Context

Solution

Size rationale

See also

bungle Mar 19, 2018

Choose a reason for hiding this comment

thibaultcha Mar 19, 2018

Choose a reason for hiding this comment

bungle Mar 19, 2018

Choose a reason for hiding this comment

thibaultcha Mar 19, 2018

Choose a reason for hiding this comment

thibaultcha Mar 19, 2018

Choose a reason for hiding this comment

thibaultcha commented Mar 17, 2018 •

edited

Loading